[RSDK-10385] Windows build system improvements and rust_utils workarounds #402
Conversation
| @@ -1,3 +1,9 @@ | |||
| #ifdef _WIN32 | |||
| #define NOMINMAX | |||
| #include <winsock2.h> | |||
There was a problem hiding this comment.
I don't love this. I'm surprised the gRPC headers don't handle this for themselves, but without this this file doesn't compile.
The right way to do this would be with a global config precompiled header that got pulled into every .cpp we have, but I don't want to spend time on that right now, so I will file a follow-up ticket.
There was a problem hiding this comment.
src/viam/sdk/CMakeLists.txt
Outdated
| @@ -132,6 +132,7 @@ target_sources(viamsdk | |||
| rpc/dial.cpp | |||
| rpc/server.cpp | |||
| rpc/private/viam_grpc_channel.cpp | |||
| rpc/private/viam_rust_utils_stubs.cpp | |||
There was a problem hiding this comment.
instead of doing an #if WIN32 in the cpp file, what if we did this at the CMake level:
if (NOT WIN32)
target_link_libraries(viamsdk
PRIVATE viam_rust_utils
)
else()
target_sources(viamsdk PRIVATE rust_utils_stubs.cpp)
endif()
There was a problem hiding this comment.
Sure, I'll give that a try, and as long as it works (which I expect it will) I'll merge with that.
There was a problem hiding this comment.
I think this looks fine assuming the main tests pass. Conan is failing but it's only as broken as it already was on main (which seems to have gotten worse recently) so not a blocker for merging.
Probably want to look into github CI for the conan package at some point however
__cplusplusis set right on Windows builds.viam_rust_utilson Windows.viam_rust_utilsentry points toaborton Windows.viamapilibrary name on Windows and apply/WHOLEARCHIVEstd::invoke_result_tinstead ofresult_ofin C++17, where the latter is deprecated